Skip to content

Fix namespace hardcode #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix namespace hardcode #148

wants to merge 1 commit into from

Conversation

0zd0
Copy link

@0zd0 0zd0 commented Jun 27, 2025

When we prefix namespaces with https://github.com/BrianHenryIE/strauss problems arise

Copy link
Member

@bsweeney bsweeney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does strauss automatically handle namespace references in use statements? What about the strings containing \FontLib\Table\... references around line 400 in this file?

@bsweeney
Copy link
Member

If the issue is around construction of a namespace in a string, you might also look at the Dompdf projecthttps://github.com/[dompdf](https://github.com/dompdf).

@0zd0
Copy link
Author

0zd0 commented Jun 29, 2025

If the issue is around construction of a namespace in a string, you might also look at the Dompdf project[https://github.com/dompdf](https://github.com/%5Bdompdf%5D(https://github.com/dompdf)).

this is exactly related to dompdf. with this fix everything started working for us and it seems like there are no errors yet

@bsweeney
Copy link
Member

I'm curious why only these lines prove problematic. There is similar logic in elsewhere in FontLib as well as in Dompdf that builds a string containing a class reference. I would expect similar issues there. I suppose it's possible you're not hitting those in this library and that only FontLib is impacted because another dependency in your project is using the "FontLib" namespace.

@0zd0
Copy link
Author

0zd0 commented Jun 29, 2025

I'm curious why only these lines prove problematic. There is similar logic in elsewhere in FontLib as well as in Dompdf that builds a string containing a class reference. I would expect similar issues there. I suppose it's possible you're not hitting those in this library and that only FontLib is impacted because another dependency in your project is using the "FontLib" namespace.

Maybe. There are no more moments like this in this library?

@bsweeney
Copy link
Member

bsweeney commented Jun 29, 2025

Maybe. There are no more moments like this in this library?

In the file you modified there's this block:

if (!self::$raw) {
  $name_canon = preg_replace("/[^a-z0-9]/", "", strtolower($tag));

  $class = "FontLib\\Table\\Type\\$name_canon";

  if (!isset($this->directory[$tag]) || !@class_exists($class)) {
    return;
  }
}
else {
  $class = "FontLib\\Table\\Table";
}

and in Dompdf there's a few, like this one:

$decorator  = "Dompdf\\FrameDecorator\\$decorator";
$reflower   = "Dompdf\\FrameReflower\\$reflower";

I'm fine to accept the change you made and wait to resolve others when they crop up. I'm not familiar with the tool you're using so I don't know what best practice would be to prevent potential rename misses. And I'm not planning to spend a lot of time researching it since it's not a primary usage scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants